Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(baseapp): fix BaseApp.getContext data races #18383

Merged
merged 1 commit into from
Nov 7, 2023
Merged

fix(baseapp): fix BaseApp.getContext data races #18383

merged 1 commit into from
Nov 7, 2023

Conversation

odeke-em
Copy link
Collaborator

@odeke-em odeke-em commented Nov 7, 2023 •
edited by coderabbitai bot
Loading

This change adds a mutex to BaseApp and firstly locks it when invoking (*BaseApp).getContextForTx. Ideally the (types/sdk).Context should be concurrency safe mirroring the Go standard library's context.Context.

Fixes #18382

Summary by CodeRabbit

  • Bug Fixes
    • Addressed a data race issue for improved stability.
    • Fixed a builder function for missed blocks key in slashing/migration/v4.
  • Improvements
    • Enhanced consistency in setting viper prefix in client and server.
    • Removed hardcoded gRPC address to localhost in the server for better flexibility.
  • Refactor
    • Modified the ExecuteGenesisTx method to a pointer receiver for better state management.

@odeke-em odeke-em marked this pull request as ready for review November 7, 2023 05:30
@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 7, 2023 •
edited
Loading

Walkthrough

The changes primarily focus on improving the Cosmos SDK by addressing a data race issue in BaseApp.getContext, setting a consistent viper prefix in the client and server, removing a hardcoded gRPC address to localhost in the server, and fixing a builder function for missed blocks key in slashing/migration/v4. The BaseApp struct has been enhanced with a sync.Mutex field named mu for concurrent access protection, and the getContextForTx function has been modified to use this mutex.

Changes

File Change Summary
baseapp/baseapp.go Introduced a sync.Mutex field named mu to the BaseApp struct for concurrent access protection. Modified the getContextForTx function to use this mutex.
baseapp/genesis.go Changed the receiver of the ExecuteGenesisTx method from BaseApp to *BaseApp, making it a pointer receiver.
CHANGELOG.md Documented the bug fixes and improvements to the Cosmos SDK, including the data race fix in BaseApp.getContext, consistent viper prefix setting, removal of hardcoded gRPC address, and the fix for the builder function in slashing/migration/v4.

Assessment against linked issues (Beta)

Objective Addressed Explanation
Fix the data race in BaseApp.getContext (#18382) ✅ The introduction of the sync.Mutex field and its use in getContextForTx function addresses this objective.
Set viper prefix consistently in client and server (#18382) ❓ The changes summary mentions this fix, but the specific changes are not visible in the provided diffs.
Remove hardcoded gRPC address to localhost in the server (#18382) ❓ The changes summary mentions this fix, but the specific changes are not visible in the provided diffs.
Fix the builder function for missed blocks key in slashing/migration/v4 (#18382) ❓ The changes summary mentions this fix, but the specific changes are not visible in the provided diffs.

Poem

🐇 Hopping through the code, with a mutex in our stride,
Fixing data races, with a sense of pride.
No more hardcoded addresses, in our server's town,
With every bug we squash, we earn our coder's crown.
As the leaves of November fall, we continue to evolve,
In the world of Cosmos SDK, mysteries we solve. 🍁👑

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@odeke-em odeke-em requested a review from a team as a code owner November 7, 2023 05:30
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
coderabbitai[bot]
coderabbitai bot reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between fb9dadc and 4bc26ca4061b2d5b7344e1469f7d7ea5e55b5265.
Files selected for processing (1)
  • baseapp/baseapp.go (3 hunks)
Additional comments: 3
baseapp/baseapp.go (3)

  • 9-9: The sync package is imported to use the sync.Mutex type for concurrency control.

  • 63-63: A new sync.Mutex field, mu, is added to the BaseApp struct to protect certain fields during concurrent access.

  • 646-648: The getContextForTx function now acquires the mutex mu at the start and ensures it is released when the function exits. This makes the function's operations atomic, preventing data races.

@odeke-em odeke-em force-pushed the baseapp-fix-getContext-data-race branch from 4bc26ca to af66258 Compare November 7, 2023 05:33
@odeke-em odeke-em mentioned this pull request Nov 7, 2023
Merged
coderabbitai[bot]
coderabbitai bot reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between fb9dadc and af66258a25f5bbb5686e6a7ad9adeb7732b6260d.
Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • baseapp/baseapp.go (3 hunks)
Additional comments: 4
CHANGELOG.md (1)
  • 75-78: The changes in the changelog accurately reflect the updates made in the pull request. Ensure that the links to the pull requests are correct and lead to the appropriate changes.
baseapp/baseapp.go (3)

  • 6-12: The sync package is imported to use sync.Mutex for thread safety.

  • 62-66: A new mutex mu is added to the BaseApp struct to protect certain fields during concurrent access.

  • 643-651: The getContextForTx function now uses the mu mutex to ensure thread safety during concurrent access. The mutex is locked at the start of the function and deferred to unlock at the end, ensuring it will be unlocked even if the function exits early.

app.mu.Lock()
defer app.mu.Unlock()

likhita-809
likhita-809 reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a failing test. I think mutexes should be operated through pointers receivers to ensure synchronized behavior

@odeke-em
fix(baseapp): fix BaseApp.getContext data races
95f365e 
This change adds a mutex to BaseApp and firstly locks it
when invoking (*BaseApp).getContextForTx. Ideally the (types/sdk).Context
should be concurrency safe mirroring the Go standard library's
context.Context.

Fixes #18382
@odeke-em odeke-em force-pushed the baseapp-fix-getContext-data-race branch from af66258 to 95f365e Compare November 7, 2023 10:03
@odeke-em
Copy link
Collaborator Author

odeke-em commented Nov 7, 2023

There is a failing test. I think mutexes should be operated through pointers receivers to ensure synchronized behavior

Thanks @likhita-809 for the feedback, please take a look! Sadly a consequence of our coding style of not sticking to pointer receivers is such test failures. I've modified BaseApp.ExecuteGenesisTx to be a pointer receiver. Tnank you @tac0turtle for the review!

coderabbitai[bot]
coderabbitai bot reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 7e00255 and 95f365e.
Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • baseapp/baseapp.go (3 hunks)
  • baseapp/genesis.go (1 hunks)
Additional comments: 5
CHANGELOG.md (1)
  • 75-78: The changes in the changelog are well-documented and provide clear information about the bug fixes and improvements made in the Cosmos SDK. The links to the pull requests provide a good reference for further details.
baseapp/genesis.go (1)
  • 12-18: The change from BaseApp to *BaseApp as the receiver of ExecuteGenesisTx is appropriate as it allows the method to modify the state of the BaseApp instance. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.
baseapp/baseapp.go (3)

  • 9-9: The sync package is imported to use sync.Mutex for thread safety.

  • 63-63: A sync.Mutex named mu is added to the BaseApp struct to protect certain fields during concurrent access.

  • 646-648: The getContextForTx function now acquires and releases the mutex to ensure safe concurrent access to certain fields.

+	app.mu.Lock()
+	defer app.mu.Unlock()

@odeke-em odeke-em added this pull request to the merge queue Nov 7, 2023
Merged via the queue into main with commit c1b3916 Nov 7, 2023
56 of 59 checks passed
@odeke-em odeke-em deleted the baseapp-fix-getContext-data-race branch November 7, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@julienrbrt julienrbrt julienrbrt approved these changes

@tac0turtle tac0turtle tac0turtle approved these changes

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@likhita-809 likhita-809 Awaiting requested review from likhita-809

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

baseapp, tests/e2e: flaky and racy code
4 participants
@odeke-em @tac0turtle @julienrbrt @likhita-809